fix(server): honor rejected identity checks during port reclaim - #4036

No actionable comments were generated in the recent review. 🎉

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

✅ Deterministic PR hygiene checks passed.

@coderabbitai review

Please review head 24c068326cb61a57babbb0a143dd62f329a17baf against 29bb221c3cfad89e4920ac411c9681073929c152, especially the stricter allowlist contract.

Port reclamation now honors a rejected OCX verifier result even for a captured teardown PID. The allowlist narrows candidates but cannot override rejection. Rejected live holders also prevent TCP-row cleanup; subsequent scans can proceed after accepted verification or an observed exit. Existing positive verification, kill flags, and dead/respawn handling remain intact. The tradeoff is retaining an unverifiable legitimate holder until the bounded wait expires. The existing classification cache is unchanged, so this is not a claim of fresh process-instance verification or complete PID-reuse protection.

Seven focused cases failed before the fix. The complete reclaim file now passes 28 tests / 69 assertions; ten existing graceful-stop/update-caller controls pass another 26 assertions. Process and TCP operations are mocked. Typecheck, privacy scan, diff check, and the 425-page documentation build passed. Independent read-only review found no required corrections. Full contributor CI and maintainer review are still pending.

⏳ DRAFT

• review readiness checklist open (2/4 boxes ticked).

What to do

• Tick all four boxes in the PR description once you're done (currently 2/4).

Review readiness checklist

• ⬜ All CI tests are green on my local testing.
• ✅ I pushed my PR to the latest dev commit.
• ✅ I resolved all correct Codex and CodeRabbit findings.
• ⬜ My PR is ready for review.

2/4 boxes ticked.

This PR stays in draft until every box above is ticked.

#!/bin/bash
set -euo pipefail

base='29bb221c3cfad89e4920ac411c9681073929c152'
head='24c068326cb61a57babbb0a143dd62f329a17baf'

printf '%s\n' '== Commit relationship =='
git merge-base "$base" "$head"
git diff --check "$base" "$head"
printf '%s\n' '== Changed files =='
git diff --name-status "$base" "$head"
printf '%s\n' '== Diff =='
git diff --find-renames --find-copies --unified=80 "$base" "$head"

printf '%s\n' '== Candidate source and test files =='
fd -t f 'port-reclaim|process-control-graceful|update-job' . || true
printf '%s\n' '== Outlines =='
while IFS= read -r file; do
  ast-grep outline "$file" --items all --view expanded || true
done < <(fd -t f 'port-reclaim|process-control-graceful|update-job' .)

#!/bin/bash
set -euo pipefail

printf '%s\n' '== port-reclaim implementation (numbered) =='
nl -ba src/server/port-reclaim.ts

printf '%s\n' '== focused test symbols =='
ast-grep outline tests/server/port-reclaim.test.ts --items all --view expanded || true
printf '%s\n' '== focused test cases (numbered) =='
nl -ba tests/server/port-reclaim.test.ts

printf '%s\n' '== reclaimListenPort call sites and surrounding code =='
rg -n -C 12 -P '\breclaimListenPort\s*\(' --glob '*.ts' .

printf '%s\n' '== option names at reclaim call sites =='
rg -n -C 8 -P '\b(killOcxHolders|onlyKillPids|killAllOcxOnPort|dropTcpRows)\s*:' --glob '*.ts' src tests

printf '%s\n' '== process identity verifier contract =='
rg -n -C 16 -P '\bfunction\s+verifyPidIdentity\b|\bverifyPidIdentity\s*=' --glob '*.ts' src

#!/bin/bash
set -euo pipefail

numbered() {
  awk '{ printf "%6d\t%s\n", NR, $0 }' "$1"
}

printf '%s\n' '== port-reclaim implementation (numbered) =='
numbered src/server/port-reclaim.ts

printf '%s\n' '== port-reclaim focused tests (numbered) =='
numbered tests/server/port-reclaim.test.ts

printf '%s\n' '== reclaimListenPort call sites and surrounding code =='
rg -n -C 12 -P '\breclaimListenPort\s*\(' --glob '*.ts' .

printf '%s\n' '== option names at reclaim call sites =='
rg -n -C 8 -P '\b(killOcxHolders|onlyKillPids|killAllOcxOnPort|dropTcpRows)\s*:' --glob '*.ts' src tests

printf '%s\n' '== verifyPidIdentity definition and direct uses =='
rg -n -C 20 -P '\b(verifyPidIdentity|function\s+verifyPidIdentity)\b' --glob '*.ts' src/config src/server src/lib

#!/bin/bash
set -euo pipefail

base='29bb221c3cfad89e4920ac411c9681073929c152'

printf '%s\n' '== Baseline reclaim loop =='
git show "${base}:src/server/port-reclaim.ts" | sed -n '180,310p'

printf '%s\n' '== Head reclaim loop =='
sed -n '177,289p' src/server/port-reclaim.ts

printf '%s\n' '== Direct termination and TCP-drop calls in reclaim module =='
rg -n -C 5 -P '\b(killFn|dropTcpFn)\s*\(' src/server/port-reclaim.ts

printf '%s\n' '== Updated test declarations =='
rg -n -P '^\s*test(?:\.each)?\(' tests/server/port-reclaim.test.ts

printf '%s\n' '== References to old allowlist-override terminology =='
rg -n -i -C 3 -P 'allowlisted.*(fail|reject)|fail.*allowlisted|trusted teardown' \
  src/server/port-reclaim.ts tests/server/port-reclaim.test.ts docs-site/src/content/docs/reference/cli/lifecycle.md docs-site/src/content/docs/ko/reference/cli/lifecycle.md structure/01_runtime.md || true

@luvs01 Reviewed 24c068326cb61a57babbb0a143dd62f329a17baf against 29bb221c3cfad89e4920ac411c9681073929c152.

I found no actionable issues.

The stricter allowlist contract is enforced in src/server/port-reclaim.ts:

• At Line 229, verification requires verifyOcxFn(pid) === pid.
• At Lines 231-235, every live verification rejection sets foreignLive, including an allowlisted PID and a killAllOcxOnPort candidate.
• At Lines 243-255, the pre-kill check still requires a second matching verification result.
• At Lines 268-284, foreignLive blocks dropTcpFn, so rejected live holders also prevent TCP-row cleanup for that scan.
• Later scans do not retain a rejection state. A later accepted verification can proceed through the normal kill path.

The caller contracts remain consistent:

• src/lib/process-control.ts restricts graceful-stop recovery to the captured PID.
• src/update/job.ts retains killAllOcxOnPort: true for update restart and still requires verifier acceptance.
• src/cli/index.ts retains killOcxHolders: false for pinned starts.

tests/server/port-reclaim.test.ts adds focused coverage for persistent rejection, failed pre-kill revalidation, mismatched verifier PIDs under both killAllOcxOnPort settings, later verifier acceptance, TCP cleanup blocking, and the existing dead/respawn behavior.

This was a static review. I did not run the test suite in the sandbox.

리뷰 · 우선순위 62 / 80

이 PR은 중지·업데이트 뒤 포트 회수(src/server/port-reclaim.ts의 reclaimListenPort)에서 예전에 적어 둔 PID allowlist가 OCX 신원 확인 실패를 덮어쓰던 구멍을 막는 버그픽스입니다. 지금 dev HEAD 7dc7dc99e에서는 살아 있는 후보가 verifyOcxFn(pid) === pid에 실패해도 onlyKillPids에 들어 있으면 kill 경로로 들어가고, 그 뒤 TCP 행 정리까지 이어질 수 있습니다. 주석도 “Pre-update PIDs can fail verify… Allowlisted teardown PIDs may be killed”라고 그 의도를 적어 두었습니다. 이 PR은 그 분기를 지우고, 검증이 거절된 살아 있는 holder는 죽이지 않고 TCP cleanup도 막는다로 바꿉니다. 나중에 같은 PID가 검증을 통과하거나 프로세스가 실제로 죽으면 기존 스캔 루프가 다시 회수를 시도합니다.

왜 지금 dev 기준으로 보면 가치가 있을까요. tip은 #4037 docs 기록이고, 그 아래 L2–L6 공급자 스택(#4026–#4031)은 이미 ancestry에 있습니다. 그와 별개로 Windows stop/update 후 포트가 붙잡히거나, 반대로 검증 실패한 PID를 잘못 죽이는 문제는 lifecycle 신뢰와 직결됩니다. 관련 문서도 HEAD의 docs-site/.../reference/cli/lifecycle.md(영·한)와 structure/01_runtime.md에 한 단락을 더해, “검증 못 하면 포트가 바쁠 수 있으니 직접 확인 후 재시도”라는 트레이드오프를 사용자에게 드러냅니다. 테스트는 tests/server/port-reclaim.test.ts에서 거절 유지·TCP 미드롭·다른 verifier PID·나중에 통과하면 회수 같은 케이스를 mock clock으로 고정했고, 작성자 보고로는 28/69 assertions 통과입니다. types.ts/config.ts 대분할과 무관해서 close-don't-rebase 대상이 아닙니다.

점수가 60대인 이유는 실제 안전(잘못된 kill) 쪽이고 디프도 port-reclaim.ts + 테스트 + docs로 좁기 때문입니다. 다만 GitHub상 draft이고, 의도적으로 “검증 안 된 leftover가 포트를 붙잡은 채 deadline까지 갈 수 있다”를 고른 계약 변경이라 메인테이너가 그 트레이드오프를 받아들여야 합니다. 기존 killOcxHolders / onlyKillPids / killAllOcxOnPort / 사전 kill 재검증 경로는 유지한다고 본문에 적혀 있습니다.

라인 port-reclaim.ts allowlisted+!isOcx kill 분기 - HEAD의 “allowlist면 verify 실패해도 kill”을 제거한 방향이 맞다. 거절된 live holder를 보호하는 쪽이 안전하다
경로/심볼 verifyOcxFn === pid - allowlist는 후보를 좁히는 용도일 뿐, 거절 결과를 뒤집지 않는다는 주석·동작이 일관되어야 한다
경로/심볼 tests/server/port-reclaim.test.ts reclaimWithMockClock - 벽시계 대신 spy Date.now로 스캔/deadline을 고정한 점은 CI flake를 줄이는 좋은 패턴이다
경로/심볼 lifecycle.md(영·한) + structure/01_runtime.md - 포트가 바쁠 수 있다는 UX를 문서에 남긴 것은 계약 변경에 필수다
경로/심볼 draft 상태 - ready·전체 CI 채우기 전에는 landing 트레인에 넣지 말 것

메인테이너의 판단이 필요한 지점

• “검증 실패 PID는 절대 안 죽인다” vs “신뢰된 teardown allowlist는 best-effort kill” 중 어떤 계약을 dev에 고정할지
• 포트가 deadline까지 busy로 남는 케이스가 Windows 업데이트/서비스 respawn에서 체감 회귀가 될지
• draft를 ready로 올리기 전에 Cross-platform CI 전체를 필수로 둘지

너의 추천
내용상 머지 가치가 높다. 작성자가 draft를 풀고 contributor CI가 그린이면 dev에 단독 버그픽스로 받아도 됩니다. 머지 전에 “allowlist가 verify를 덮지 않는다”는 계약을 MAINTAINERS/구조 문서 한 줄과 맞는지 한 번만 확인하세요. 닫을 중복 원본은 없습니다.

이 댓글은 grok-bot이 작성했습니다

@coderabbitai review

Please confirm the reviewed result for current head a4a87b70f4d865af53892733560b23b6dd23e792 after rebasing onto dev 7dc7dc99e65268bc8764e19840952256b030bce9 to satisfy the readiness gate's live-base limit. All five PR files are byte-identical to previously reviewed 24c068326; the 22 upstream commits touched none of them. Typecheck and diff check passed after rebasing.

The prior contributor CI remains failed evidence: the unchanged idleDeadline reset/postpone test was the only test failure in the macOS control. It expected four 40 ms waits to resume within successive 120 ms idle windows. The callback had fired before the assertion (432.21 ms test duration); exact per-callback timing was not logged. The primitive uses setTimeout/clearTimeout, not Date.now, and the port fixture restores its clock spy in finally. Current-head contributor CI is being run once after the base alignment. No runtime assertion or unrelated test was changed to hide this failure, and the PR remains a draft pending green CI and the maintainer's allowlist-contract judgment.

#!/bin/bash
set -euo pipefail

old_base='29bb221c3cfad89e4920ac411c9681073929c152'
old_head='24c068326cb61a57babbb0a143dd62f329a17baf'
new_base='7dc7dc99e65268bc8764e19840952256b030bce9'
new_head='a4a87b70f4d865af53892733560b23b6dd23e792'

printf '%s\n' '== Commit availability and ancestry =='
git cat-file -e "${old_base}^{commit}"
git cat-file -e "${old_head}^{commit}"
git cat-file -e "${new_base}^{commit}"
git cat-file -e "${new_head}^{commit}"
printf 'new merge-base: '
git merge-base "$new_base" "$new_head"
printf 'old merge-base: '
git merge-base "$old_base" "$old_head"

printf '%s\n' '== Diff checks =='
git diff --check "$old_base" "$old_head"
git diff --check "$new_base" "$new_head"

printf '%s\n' '== Changed paths (old and new) =='
git diff --name-status "$old_base" "$old_head"
printf '%s\n' '--'
git diff --name-status "$new_base" "$new_head"

printf '%s\n' '== Patch identity =='
git diff --binary --full-index "$old_base" "$old_head" > /tmp/old.patch
git diff --binary --full-index "$new_base" "$new_head" > /tmp/new.patch
sha256sum /tmp/old.patch /tmp/new.patch
cmp -s /tmp/old.patch /tmp/new.patch && echo 'patches: byte-identical' || echo 'patches: DIFFER'

printf '%s\n' '== Per-path postimage blobs =='
while IFS= read -r path; do
  old_blob=$(git rev-parse "${old_head}:${path}")
  new_blob=$(git rev-parse "${new_head}:${path}")
  printf '%s\t%s\t%s\t' "$path" "$old_blob" "$new_blob"
  test "$old_blob" = "$new_blob" && echo identical || echo DIFFER
done < <(git diff --name-only "$new_base" "$new_head")

printf '%s\n' '== Current implementation map =='
ast-grep outline src/server/port-reclaim.ts --items all --view expanded
printf '%s\n' '== Current contract-critical code =='
rg -n -C 10 -P '\b(verifyOcxFn|foreignLive|onlyKillPids|killAllOcxOnPort|dropTcpFn|killFn)\b' src/server/port-reclaim.ts

printf '%s\n' '== Current focused test declarations =='
ast-grep outline tests/server/port-reclaim.test.ts --items all --view expanded
rg -n -C 3 -P '^\s*test(?:\.each)?\(' tests/server/port-reclaim.test.ts

#!/bin/bash
set -euo pipefail

old_head='24c068326cb61a57babbb0a143dd62f329a17baf'
new_base='7dc7dc99e65268bc8764e19840952256b030bce9'
new_head='a4a87b70f4d865af53892733560b23b6dd23e792'

paths_file=/tmp/rebased-pr-paths.txt
git diff --name-only "$new_base" "$new_head" > "$paths_file"

printf '%s\n' '== Per-path postimage blobs =='
while IFS= read -r path; do
  old_blob=$(git rev-parse "${old_head}:${path}")
  new_blob=$(git rev-parse "${new_head}:${path}")
  if [ "$old_blob" = "$new_blob" ]; then
    printf '%s\tidentical\t%s\n' "$path" "$new_blob"
  else
    printf '%s\tDIFFER\told=%s\tnew=%s\n' "$path" "$old_blob" "$new_blob"
  fi
done < "$paths_file"

printf '%s\n' '== Current port-reclaim relevant region =='
git show "${new_head}:src/server/port-reclaim.ts" | nl -ba | sed -n '180,300p'

printf '%s\n' '== Current focused test names =='
git show "${new_head}:tests/server/port-reclaim.test.ts" |
  rg -n -C 3 -P '^\s*test(?:\.each)?\('

printf '%s\n' '== Current patch statistics =='
git diff --stat "$new_base" "$new_head"
git diff --check "$new_base" "$new_head"

@luvs01 Confirmed for current head a4a87b70f4d865af53892733560b23b6dd23e792.

The merge base is exactly dev commit 7dc7dc99e65268bc8764e19840952256b030bce9.

The rebased patch is byte-identical to the previously reviewed patch:

• Both complete patches have SHA-256 e0eccf43828ec43ba0976692a1c28d595f57e7cc3a6946c73e563ecd588c12ab.
• git diff --check passed for both commit ranges.
• The same five files changed.
• Each changed file has the same postimage blob at 24c068326 and a4a87b70:
  • src/server/port-reclaim.ts
  • tests/server/port-reclaim.test.ts
  • docs-site/src/content/docs/reference/cli/lifecycle.md
  • docs-site/src/content/docs/ko/reference/cli/lifecycle.md
  • structure/01_runtime.md

Therefore, the prior static result remains valid. I found no actionable issues.

The reviewed contract remains unchanged in src/server/port-reclaim.ts:

• Line 229 requires verifyOcxFn(pid) === pid.
• Lines 231-235 treat every live verification rejection as foreignLive.
• Lines 243-255 require matching successful verification before kill.
• Lines 268-284 prevent TCP-row cleanup when foreignLive is set.
• The allowlist narrows kill candidates. It does not override rejected identity verification.

The sandbox did not run tests. The initial recheck script could not print the source region because nl is unavailable, but the exact source and test blob matches make further reinspection unnecessary.

The macOS idleDeadline failure remains external CI evidence. It is not caused by a rebase change in this PR because the focused test file and all other PR files are identical. Keep the PR draft until current-head contributor CI is green and a maintainer accepts the bounded-wait behavior for unverifiable live holders.